-
Notifications
You must be signed in to change notification settings - Fork 346
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Migrate from pytz to zoneinfo, list (close to) all time zones #3167
Conversation
I have run into at least one RFE that depends on not simplifying the zone list to the common ones. (The Montreal thing by @lsatenstein.) Unless we find some technical or usability reason to simplify the list, I would be most happy to see the full list. The main doubt I had was the ability to map the zones back onto the map-of-the-world thingy. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rebase and re-target to f34-devel? There's nothing on master so it should be no work at all.
Tests are mostly broken right now, so no point enabling them :( |
You want this in f34 already? Fine by me. |
How do I check easiest if this even runs? I see you use packit. Does it produce rpms I can install in a Fedora 34 VM? |
/test |
Before this PR: After this PR: The UI seems to do weird things, e.g. if I try to select Australia/Canberra, it selects Australia/Sydney etc. Or if I select United States, it gives me Americas instead and I need to to select United States 3 to 4 times in order to actually see the options there. Not sure if it is related to my PR or not. |
https://listman.redhat.com/archives/anaconda-devel-list/2021-February/msg00020.html Python's zoneinfo does not have any concept of "common timezones". Hence, we show all zones with slash in them, except some weird Etc ones. This effectively adds about 17% of zones, today, that means: Africa/Asmera Africa/Timbuktu America/Argentina/ComodRivadavia America/Atka America/Buenos_Aires America/Catamarca America/Coral_Harbour America/Cordoba America/Ensenada America/Fort_Wayne America/Godthab America/Indianapolis America/Jujuy America/Knox_IN America/Louisville America/Mendoza America/Montreal America/Porto_Acre America/Rosario America/Santa_Isabel America/Shiprock America/Virgin Antarctica/South_Pole Asia/Ashkhabad Asia/Calcutta Asia/Chongqing Asia/Chungking Asia/Dacca Asia/Harbin Asia/Istanbul Asia/Kashgar Asia/Katmandu Asia/Macao Asia/Rangoon Asia/Saigon Asia/Tel_Aviv Asia/Thimbu Asia/Ujung_Pandang Asia/Ulan_Bator Atlantic/Faeroe Atlantic/Jan_Mayen Australia/ACT Australia/Canberra Australia/Currie Australia/LHI Australia/NSW Australia/North Australia/Queensland Australia/South Australia/Tasmania Australia/Victoria Australia/West Australia/Yancowinna Brazil/Acre Brazil/DeNoronha Brazil/East Brazil/West Canada/Saskatchewan Canada/Yukon Chile/Continental Chile/EasterIsland Europe/Belfast Europe/Nicosia Europe/Tiraspol Mexico/BajaNorte Mexico/BajaSur Mexico/General Pacific/Johnston Pacific/Ponape Pacific/Samoa Pacific/Truk Pacific/Yap US/Aleutian US/East-Indiana US/Indiana-Starke US/Michigan US/Samoa See also rhinstaller#3167 (comment) for why is that OK.
https://listman.redhat.com/archives/anaconda-devel-list/2021-February/msg00020.html Python's zoneinfo does not have any concept of "common timezones". Hence, we show all zones with slash in them, except some weird Etc ones. This effectively adds about 17% of zones, today, that means: Africa/Asmera Africa/Timbuktu America/Argentina/ComodRivadavia America/Atka America/Buenos_Aires America/Catamarca America/Coral_Harbour America/Cordoba America/Ensenada America/Fort_Wayne America/Godthab America/Indianapolis America/Jujuy America/Knox_IN America/Louisville America/Mendoza America/Montreal America/Porto_Acre America/Rosario America/Santa_Isabel America/Shiprock America/Virgin Antarctica/South_Pole Asia/Ashkhabad Asia/Calcutta Asia/Chongqing Asia/Chungking Asia/Dacca Asia/Harbin Asia/Istanbul Asia/Kashgar Asia/Katmandu Asia/Macao Asia/Rangoon Asia/Saigon Asia/Tel_Aviv Asia/Thimbu Asia/Ujung_Pandang Asia/Ulan_Bator Atlantic/Faeroe Atlantic/Jan_Mayen Australia/ACT Australia/Canberra Australia/Currie Australia/LHI Australia/NSW Australia/North Australia/Queensland Australia/South Australia/Tasmania Australia/Victoria Australia/West Australia/Yancowinna Brazil/Acre Brazil/DeNoronha Brazil/East Brazil/West Canada/Saskatchewan Canada/Yukon Chile/Continental Chile/EasterIsland Europe/Belfast Europe/Nicosia Europe/Tiraspol Mexico/BajaNorte Mexico/BajaSur Mexico/General Pacific/Johnston Pacific/Ponape Pacific/Samoa Pacific/Truk Pacific/Yap US/Aleutian US/East-Indiana US/Indiana-Starke US/Michigan US/Samoa See also rhinstaller#3167 (comment) for why is that OK.
I think that's the map widget, just as I feared - it doesn't know where some of the time zones actually lie. Alternatively, it is concerned only with time zones, not locales, and swaps them for the most current definition. Or it is Anaconda's doing. I'm literally writing here ideas as I browse the data, so feel free to draw your own conclusions, that might be better. https://github.com/dashea/timezonemap PS: Did you swap the labels for before and after? PPS: The first Americas thing does not get a pin either way. I think the data there are just out of date. Maybe the way to go forward is to identify the gaps, and patch that thing first. Or filter what we show (shudder). |
Sorry for recurrent brain dumps, but here I go once more. I think there's some way to tell if a TZ is an alias or not - that might be the key to getting rid of the auto-switching ones. IIRC I saw code for such filtering in Anaconda, so this might be a similar but subtly different problem. |
Oh look, here we go: https://github.com/dashea/timezonemap/blob/master/src/data/backward Explains the Canberra -> Sydney at least a bit. |
@@ -136,15 +148,14 @@ def get_all_regions_and_timezones(): | |||
|
|||
result = OrderedDict() | |||
|
|||
for tz in pytz.common_timezones: | |||
for tz in sorted(all_timezones()): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be better to move the sorting into function itself to take advantage of the cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically, it could. However the other function that uses this does not need sorting and it appeared to me that get_all_regions_and_timezones()
is usually called just once? If it is called multiple times, should that be cached as well?
@hroncok Thanks for the PR! Will try to respond to individual points bellow. :)
That's a good point, I wonder if just having the extra timezones would be a problem - the list in the GUI is already long, but 17% extra zones would hardly make it worse. Also as @VladimirSlavik mentioned, there have been RFEs about missing zones. If we really need to have some custom list of zones to include or exclude for some reason, I think it should really not be hardcoded in Anaconda source code but instead live in langtable (https://github.com/mike-fabian/langtable) like all similar language/locale/keyboard mapping things we use. Still in such a case I would arrange for that, can't really burden you with that extra work. :)
Seeing the screenshots I see you got it working, but for the record - for interactive debugging like this editing the Anaconda files on the live image is likely best. It should be possible to start sshd on a booted live image and overwrite the Anaconda files as needed. The just relaunch Anaconda using the launcher icon after every edit. Alternatively it's possible to use the updates image mechanism. That's a bit more involved but can be automated, starts with clean environment every time and can be used to test the netinst image. I can get you started on this if you want. :)
Let's say that this has been fragile and full of edge cases already. :P Still I guess the only thing really changing is that some timezones might be called differently & there could be more of them. And if there are already some mappings in the timezonemap projects maybe these could be adjusted to match the new names ? |
What do I do here? |
@hroncok I'm not sure, but we'll figure it out. Unfortunately I'm not able to start digging into this right away. Assuming the widget really needs some dusting off and updating: Somebody would have to figure out what goes bonkers in that thing due to missing data, and get a solid idea what needs to be fixed there. Then we can start with the PRs... I am aware that that's far more than just rewriting bits of anaconda, so maybe that would be a task for me? |
After taking a slightly longer look at this, I think it's fine. @hroncok please rebase to master && change target branch, and I'll take care of tests. If a "link" timezone is selected, GUI changes it to the "target" one. That seems the only potential point of contention... |
https://listman.redhat.com/archives/anaconda-devel-list/2021-February/msg00020.html Python's zoneinfo does not have any concept of "common timezones". Hence, we show all zones with slash in them, except some weird Etc ones. This effectively adds about 17% of zones, today, that means: Africa/Asmera Africa/Timbuktu America/Argentina/ComodRivadavia America/Atka America/Buenos_Aires America/Catamarca America/Coral_Harbour America/Cordoba America/Ensenada America/Fort_Wayne America/Godthab America/Indianapolis America/Jujuy America/Knox_IN America/Louisville America/Mendoza America/Montreal America/Porto_Acre America/Rosario America/Santa_Isabel America/Shiprock America/Virgin Antarctica/South_Pole Asia/Ashkhabad Asia/Calcutta Asia/Chongqing Asia/Chungking Asia/Dacca Asia/Harbin Asia/Istanbul Asia/Kashgar Asia/Katmandu Asia/Macao Asia/Rangoon Asia/Saigon Asia/Tel_Aviv Asia/Thimbu Asia/Ujung_Pandang Asia/Ulan_Bator Atlantic/Faeroe Atlantic/Jan_Mayen Australia/ACT Australia/Canberra Australia/Currie Australia/LHI Australia/NSW Australia/North Australia/Queensland Australia/South Australia/Tasmania Australia/Victoria Australia/West Australia/Yancowinna Brazil/Acre Brazil/DeNoronha Brazil/East Brazil/West Canada/Saskatchewan Canada/Yukon Chile/Continental Chile/EasterIsland Europe/Belfast Europe/Nicosia Europe/Tiraspol Mexico/BajaNorte Mexico/BajaSur Mexico/General Pacific/Johnston Pacific/Ponape Pacific/Samoa Pacific/Truk Pacific/Yap US/Aleutian US/East-Indiana US/Indiana-Starke US/Michigan US/Samoa See also rhinstaller#3167 (comment) for why is that OK.
After discussing with Miro, I went ahead with the changes required to get this done. Rebased this to master, force-pushed, and retargeted. May need another force-push to re-run tests and clean up GH statuses, it's a total mess :( |
https://listman.redhat.com/archives/anaconda-devel-list/2021-February/msg00020.html Python's zoneinfo does not have any concept of "common timezones". Hence, we show all zones with slash in them, except some weird Etc ones. This effectively adds about 17% of zones, today, that means: Africa/Asmera Africa/Timbuktu America/Argentina/ComodRivadavia America/Atka America/Buenos_Aires America/Catamarca America/Coral_Harbour America/Cordoba America/Ensenada America/Fort_Wayne America/Godthab America/Indianapolis America/Jujuy America/Knox_IN America/Louisville America/Mendoza America/Montreal America/Porto_Acre America/Rosario America/Santa_Isabel America/Shiprock America/Virgin Antarctica/South_Pole Asia/Ashkhabad Asia/Calcutta Asia/Chongqing Asia/Chungking Asia/Dacca Asia/Harbin Asia/Istanbul Asia/Kashgar Asia/Katmandu Asia/Macao Asia/Rangoon Asia/Saigon Asia/Tel_Aviv Asia/Thimbu Asia/Ujung_Pandang Asia/Ulan_Bator Atlantic/Faeroe Atlantic/Jan_Mayen Australia/ACT Australia/Canberra Australia/Currie Australia/LHI Australia/NSW Australia/North Australia/Queensland Australia/South Australia/Tasmania Australia/Victoria Australia/West Australia/Yancowinna Brazil/Acre Brazil/DeNoronha Brazil/East Brazil/West Canada/Saskatchewan Canada/Yukon Chile/Continental Chile/EasterIsland Europe/Belfast Europe/Nicosia Europe/Tiraspol Mexico/BajaNorte Mexico/BajaSur Mexico/General Pacific/Johnston Pacific/Ponape Pacific/Samoa Pacific/Truk Pacific/Yap US/Aleutian US/East-Indiana US/Indiana-Starke US/Michigan US/Samoa See also rhinstaller#3167 (comment) for why is that OK. The TimezoneMap widget does not seem to have any substantial problems with this.
/kickstart-test --testtype smoke |
Not sure what the codecov problem is, it links to some cute but generic error message. |
/kickstart-test --testtype time |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot this bit.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Awesome! |
Haha, you are too. |
I think this has caused a problem with daylight savings. openQA is showing the clock exactly an hour fast on fresh installs; our official openQA deployments are in the Eastern timezone, which uses daylight savings. This is, I think, causing two tests to fail (in non-obvious ways it took me a while to trace back to the wrong time and then to this commit). These problems started in exactly the compose this commit ultimately landed in - Fedora-Rawhide-20210520.n.1. I'm looking through the changes now to see if I can spot the issue - I think I actually touched the |
OK, yeah, it's definitely this commit. Here's a test script I ginned up to compare
|
Since the port from pytz to zoneinfo in rhinstaller#3167, anaconda has set the system clock on hour too fast when installing for a timezone that does daylight savings time, during daylight savings time. This fixes it, according to my tests: we don't need to use an epoch definition forced to the timezone, we can just use an epoch definition with UTC timezone and let Python do the math. There turns out to be a lot of history to exactly how we do this calculation, see https://bugzilla.redhat.com/show_bug.cgi?id=1965718#c4 for more details. This winds up returning things to more or less exactly how they were way back before rhbz#1251044 , but now (we hope) Python and zoneinfo don't have the bugs they had back then and it'll "just work" properly. Resolves: rhbz#1965718
Since the port from pytz to zoneinfo in rhinstaller#3167, anaconda has set the system clock one hour too fast when installing for a timezone that does daylight savings time, during daylight savings time. This fixes it, according to my tests: we don't need to use an epoch definition forced to the timezone, we can just use an epoch definition with UTC timezone and let Python do the math. There turns out to be a lot of history to exactly how we do this calculation, see https://bugzilla.redhat.com/show_bug.cgi?id=1965718#c4 for more details. This winds up returning things to more or less exactly how they were way back before rhbz#1251044 , but now (we hope) Python and zoneinfo don't have the bugs they had back then and it'll "just work" properly. Resolves: rhbz#1965718
Since the port from pytz to zoneinfo in rhinstaller#3167, anaconda has set the system clock one hour too fast when installing for a timezone that does daylight savings time, during daylight savings time. There turns out to be a lot of history behind this calculation, see https://bugzilla.redhat.com/show_bug.cgi?id=1965718#c4 for details. It turns out that since Python 3.3, datetime objects have the `timestamp()` method, which does exactly what we want here. In my tests it gives the correct answer in every problematic case, including the one I found was broken in current code, and the Kolkata and Aden timezones mentioned in https://bugzilla.redhat.com/show_bug.cgi?id=1293314 . So I think we should just use that. Resolves: rhbz#1965718
Thanks for the analysis and proposed fix @AdamWill! |
Related: jira#RHEL-13150 Related: jira#RHEL-13151 Related: rhbz#1452873 Upstream solution: rhinstaller/anaconda#3167
Related: jira#RHEL-13150 Related: jira#RHEL-13151 Related: rhbz#1452873 Upstream solution: rhinstaller/anaconda#3167
Related: jira#RHEL-13150 Related: jira#RHEL-13151 Related: rhbz#1452873 Upstream solution: rhinstaller/anaconda#3167
https://listman.redhat.com/archives/anaconda-devel-list/2021-February/msg00020.html
This is very much untested. I've written it and never actually attempted to run it. Not yet anyway.
I found a problem thou. Python's
zoneinfo
does not have any concept of common timezones. I can implement my own method to determine whether a timezone is common (pytz uses zone.tab and a short list of hardcoded values).With pytz 2021.1, the extra zones effectively added by this PR are:
That's 17.6% of extra zones.
Please let me know if this actually matters or not.